PR Review: feat(studio,studio-server,cli): render cancel end-to-end + renders/nle/storyboard UX

VERDICT: APPROVE (soft) — strong work across a wide surface. Three items to resolve, one architectural suggestion.

Summary

This is the plumbing + UX half of render cancel: server route, adapter abort hooks, SSE terminal-status handling, partial-output cleanup, and a healthy pile of render queue / NLE / storyboard polish. The cancel lifecycle is well-designed — AbortController at the adapter layer, RenderCancelledError propagation through the producer, optimistic UI in the client, and cleanup of partial files so cancelled jobs don't haunt the history. The NLE and storyboard changes are largely a11y completions (APG tabs, role="separator", role="progressbar", beforeunload guard, blur autosave) and correctness fixes (composition-size-aware drop coordinates, zoom HUD per-frame updates, initial pan clamping).

Ponytail Lens

The cancel lifecycle has a clean separation of concerns: the route marks the job cancelled and invokes the hook, the adapter aborts the controller, the producer throws RenderCancelledError at the next checkpoint, and the catch block cleans up partial output. The double-status-set (route sets "cancelled", catch block re-sets it) is redundant but harmless since they're the same object reference. The executeRenderJob function already has thorough abort support — checkpoint polling via assertNotAborted() at every stage boundary, signal forwarding to parallel capture workers, and typed RenderCancelledError with resource cleanup. This PR correctly plugs into that existing infrastructure rather than reinventing it.

The hidden-ids mechanism for "Hide finished" is appropriately scoped (per-project localStorage key, capped at 200 entries to prevent unbounded growth, defensive JSON parsing). The deleteRender change from fire-and-forget to error-aware is a meaningful reliability improvement — previously a failed delete silently removed the UI row while the file stayed on disk.

Findings

[medium] Side effect inside setState updater — NLEPreview.tsx:183

setCompositionSize((prev) => {
  if (prev?.width === next?.width && prev?.height === next?.height) return prev;
  onCompositionSizeChangeRef.current?.(next);  // <-- side effect in updater
  return next;
});

State updater functions should be pure — React may call them multiple times in Strict Mode (dev) and during concurrent rendering. The onCompositionSizeChange callback sets parent state (setPreviewCompositionSize), which is idempotent, so this won't cause visible bugs in production. But it's a React anti-pattern that could confuse future maintainers or break under stricter concurrent features.

Suggestion: Extract the side effect to a useEffect that watches compositionSize:

const updateCompositionSizeFromPreview = useCallback(() => {
  const next = readPreviewCompositionSize(previewIframeRef.current);
  setCompositionSize((prev) =>
    prev?.width === next?.width && prev?.height === next?.height ? prev : next,
  );
}, []);

useEffect(() => {
  onCompositionSizeChange?.(compositionSize);
}, [compositionSize, onCompositionSizeChange]);

[low] New RenderQueue props unwired in consumer

The props onCancel, loadError, onRetryLoad, actionError, onDismissActionError are all defined as optional in RenderQueueProps and destructured in the component, but StudioRightPanel.tsx (the only consumer) doesn't pass them. The cancelRender, loadError, actionError, dismissActionError, and reloadRenders values from useRenderQueue flow into StudioContext but never reach <RenderQueue>. This means: the cancel button renders but does nothing (calls onCancel?.() which is undefined); load errors and action errors are silently swallowed in the UI.

I assume this wiring lands in a later PR in the 7-PR stack — just flagging to make sure it doesn't get lost.

[low] Missing aria-valuemax on TimelineResizeDivider

The separator declares aria-valuenow and aria-valuemin={MIN_TIMELINE_H} but omits aria-valuemax. Assistive tech can't communicate the full range without it. The max is dynamic (containerHeight - MIN_PREVIEW_H), so it would need computing at render time — an easy addition:

aria-valuemax={Math.round(
  (containerRef.current?.getBoundingClientRect().height ?? 600) - MIN_PREVIEW_H
)}

[nit] Redundant setIsDragOver(true) in handleDragOver

With the new handleDragEnter setting setIsDragOver(true), the same call in handleDragOver (line ~72 in usePreviewBlockDrop.ts) is now redundant. It doesn't cause bugs (browsers fire dragenter before dragover), but removing it would make the intent clearer — dragenter/dragleave own the flag, dragover only handles preventDefault.

Diff Stats

Review by Miga

🤖 Generated with Claude Code

Stack review for PR 2/7.

Audited: packages/studio-server/src/routes/render.ts, render.test.ts, packages/studio/src/components/renders/useRenderQueue.ts, RenderQueue.tsx, RenderQueueItem.tsx, NLEPreview.tsx, and TimelineResizeDivider.tsx at head b143798.

The server-side cancel path is narrow and test-backed: it marks rendering jobs terminal, invokes the adapter cancel hook, and terminates progress streaming on any non-rendering state. Client-side deletion/cancel error surfacing is materially better than the prior silent removal path, and the render history “hide, not delete” persistence is scoped per project and capped.

One stack-boundary caveat: at this PR boundary, RenderQueue’s onCancel/error props are introduced before StudioRightPanel wires them, so the cancel button is only truly end-to-end once #1964 lands. I verified #1964 performs that wiring. I’m approving this as part of the seven-PR Graphite stack, not as a standalone release boundary. Miga’s note on the side effect inside the NLEPreview state updater is also real but non-blocking here.

Verdict: APPROVE
Reasoning: The cancel/server/client primitives are sound, and the only functional incompleteness is resolved by the immediately-upstack #1964 wiring that is part of this requested stack.

— Magi

Current-head re-review at 18e87aceea0d8375618b1d0d16bc535c32ce8617 after Vance's follow-up.

Audited: packages/studio/src/components/nle/NLEPreview.tsx, TimelineResizeDivider.tsx, usePreviewBlockDrop.ts, packages/studio/src/components/renders/useRenderQueue.ts, packages/studio/vite.adapter.ts, and packages/cli/src/server/studioServer.ts at the current head. The NLEPreview composition-size callback has moved out of the state updater into an effect, the timeline separator now declares aria-valuemax, drag-over no longer redundantly owns the hover flag, and the cancel path now reconciles server status plus protects against cancel/complete races in both adapters.

Code checks are green; Graphite mergeability is still pending because this is an upstack PR.

Verdict: APPROVE
Reasoning: The current-head delta resolves the earlier review notes and strengthens the cancel race behavior without introducing a new release blocker; remaining merge gating is stack/Graphite state, not a code finding.

— Magi

Reviewed at 7174f29.

Note on SHA drift: Magi re-approved at 18e87ace — current merged HEAD is 7174f298, one rebase forward (onto main after #1962 landed as cd1adcb5). Diff since Magi's re-review is the merge context only; no PR-file drift in packages/studio-*, packages/cli/src/server/*, or the render route.

Second-pass lens after Miga's soft-approve rollup + Magi's re-approve at 18e87ac. Confirming Miga's two open items are addressed at HEAD: (a) NLEPreview.tsx:179-190 — the composition-size updater is now pure (setCompositionSize((prev) => …)), and the parent notification lives in a follow-up useEffect keyed on compositionSize; Strict-Mode double-fire path is closed. (b) TimelineResizeDivider.tsx aria-valuemax is present, and both adapters now guard the cancel/complete race (vite.adapter.ts:261-266 + cli/src/server/studioServer.ts:429-434 — if (signal.aborted) { removeCancelledOutput(); return; } on both the success return path and the catch path).

Two net-new items I didn't see flagged upthread:

Cancel telemetry is silent — 🟠 should-fix (post-merge follow-up)

The PR body explicitly notes cancels "skip error telemetry" — correct choice, cancels aren't failures. But nothing emits a cancel event either. packages/cli/src/server/studioRenderTelemetry.ts ships emitStudioRenderComplete (line 106) + emitStudioRenderError (line 84) but no emitStudioRenderCancel. Both adapters (cli/src/server/studioServer.ts:429-434, studio/vite.adapter.ts:261-266) return early on the abort branch without emitting anything.

Downstream effect: PostHog will show studio_render_start counts that exceed render_complete + render_error{source:"studio"}, and there is no way to close that gap — you can't distinguish "user cancelled mid-render" from "server crashed mid-render" from "browser tab closed" in the data. Given the PR title is render cancel end-to-end, closing the observability loop feels in-scope. Suggest a trackRenderCancel (or reuse trackRenderError with a distinguished errorCode: "user_cancelled") called from the abort branch in both adapters.

End-to-end claim isn't backed by a client-cancel test — 🟡 nit

The three new tests in packages/studio-server/src/routes/render.test.ts:432-505 cover the server route well — cancel marks status, invokes adapter hook, 404 for unknown, no-op if job already complete. Good.

But the client-side cancel path — useRenderQueue.ts:cancelRender (line 268-298), which does optimistic UI + closes SSE + reconciles with the server's authoritative status on res.ok — has no unit tests. The reconcile branch (res.ok && body.status !== "cancelled" → void loadRenders()) is the most valuable piece of that function because it's what saves the user from a stuck-optimistic row when the render actually finished as the cancel POST was in flight, and it's the only piece not verifiable from the route tests. A renderHooks test around useRenderQueue covering (a) cancel with SSE mid-flight, (b) reconcile after a race-completed status, (c) !res.ok → actionError set — would back the "end-to-end" claim.

Non-findings (verified good)

• Cancel/complete race — properly preempted in both adapters via abortController.signal.aborted check on both success return AND catch branches.
• Partial-output cleanup — removeCancelledOutput unlinks both video + .meta.json; renderJobs map keeps the state so SSE readers still see terminal status; cleanupFinishedJobs in render.ts:26-37 sweeps by job.status !== "rendering" so cancelled entries TTL out correctly.
• SSE terminates cleanly on cancel — client closes on terminal status (useRenderQueue.ts:237-239), server loop exits on next poll iteration when current.status !== "rendering" (render.ts:145).
• Cancelled files stay off the on-disk history — render.ts:294-303 only registers .mp4/.webm/.mov files that exist, and cancelled runs unlink both the media + meta, so loadRenders doesn't resurrect them post-restart.

Verdict: LGTM from my side — leaving as COMMENT since Magi already stamped. Cancel-telemetry gap is worth a small follow-up PR.

— Review by Rames D Jusso